[Woo POS][Local catalog] Create local database when POS tab is shown #16054
Conversation
joshheald
added
type: task
|
|
jaclync
left a comment
jaclync
left a comment
There was a problem hiding this comment.
LGTM, tested both feature flag states and it worked as expected 
| self.databasePath = databasePath | ||
| public final class GRDBManager: GRDBManagerProtocol { | ||
|
|
||
| public var databaseConnection: GRDBDatabaseConnection |
There was a problem hiding this comment.
nit: could this be let? do we plan to change the connection once it's initialized?
| self.databaseQueue = try DatabaseQueue(path: databasePath) | ||
| self.databaseConnection = try DatabaseQueue(path: databasePath) |
There was a problem hiding this comment.
just curious, is this renaming to make it more flexible if we ever decide to switch to DatabasePool?
There was a problem hiding this comment.
Yes, and to make it mockable – the protocol requirement is DatabaseReader & DatabaseWriter, which both DatabasePool and DatabaseQueue implement. GRDB refers to them as database connections for the general case.
|
|
||
| guard let grdbManager = _grdbManager else { | ||
| do { | ||
| let documentsPath = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first! |
There was a problem hiding this comment.
nit: maybe we can fatalError with a custom message instead of direct crashing from implicitly unwrapping with !
|
|
||
| /// Local SQLite Database Name | ||
| /// | ||
| static let localSQLiteDatabaseName = "woo-local.db" |
There was a problem hiding this comment.
nit: any reasons not to use the *.sqlite extension as in the example in the doc? might we want to name it like WooCommerce-local just to be more consistent with the existing sqlite files from Core Data?
I was still able to open the database in DataGrip with woo-local.db in the directory, so just a non-blocking question.
There was a problem hiding this comment.
No, I'd put more thought into the name than the extension. .sqlite is more descriptive, let's use that.
We could go to WooCommerce-local – I didn't, in order to put a bit more space between the Core Data name and the GRDB name. But I'm not holding that view super strongly. I'll leave it as it is for now, but if you feel strongly just let me know and I'll change it 😊
joshheald
force-pushed
the
woomob-1087-woo-poslocal-catalog-create-database-when-the-app-starts
branch
from
8371993 to
9790bd8
Compare
joshheald
deleted the
woomob-1087-woo-poslocal-catalog-create-database-when-the-app-starts
branch
Description
This PR creates or connects to the local catalog database when the POS tab is shown.
Steps to reproduce
Started GRDBManager with database path: /var/mobile/Containers/Data/Application/8A3E77E9-E527-4D74-87E5-15F91E5DA427/Documents/woo-local.dbis logged, along with the schemaRepeat with pointOfSaleLocalCatalogi1 feature flag set to
false– observe those are not logged.Testing information
Screenshots
RELEASE-NOTES.txtif necessary.